Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

STR-482 improved master xpriv resolution #705

Merged
merged 1 commit into from
Mar 12, 2025
Merged

STR-482 improved master xpriv resolution #705

merged 1 commit into from
Mar 12, 2025
+60 −55

Conversation

delbonis
Copy link
Contributor

@delbonis delbonis commented Mar 3, 2025

Description

This just reworks how we resolve the master xpriv to clean it up a bit and add very loud errors in the right places.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature/Enhancement (non-breaking change which adds functionality or enhances an existing one)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactor
  • New or updated tests
  • Dependency Update

Checklist

  • I have performed a self-review of my code.
  • I have commented my code where necessary.
  • I have updated the documentation if needed.
  • My changes do not introduce new warnings.
  • I have added (where necessary) tests that prove my changes are effective or that my feature works.
  • New and existing tests pass with my changes.

Related Issues

  • STR-482

@delbonis delbonis requested review from a team as code owners March 3, 2025 21:50
@codecov Codecov
Copy link

codecov bot commented Mar 3, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 26 lines in your changes missing coverage. Please review.

Project coverage is 54.96%. Comparing base (83ee94d) to head (80605e4).
Report is 25 commits behind head on main.

Files with missing lines Patch % Lines
bin/bridge-client/src/xpriv.rs 0.00% 19 Missing ⚠️
bin/bridge-client/src/modes/operator/bootstrap.rs 0.00% 7 Missing ⚠️ 
@@            Coverage Diff             @@
##             main     #705      +/-   ##
==========================================
- Coverage   54.97%   54.96%   -0.01%     
==========================================
  Files         315      315              
  Lines       34517    34512       -5     
==========================================
- Hits        18975    18969       -6     
- Misses      15542    15543       +1
Files with missing lines Coverage Δ
bin/bridge-client/src/modes/operator/bootstrap.rs 0.00% <0.00%> (ø)
bin/bridge-client/src/xpriv.rs 0.00% <0.00%> (ø)

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Mar 3, 2025

Commit: 03fa2ad

SP1 Performance Test Results

program cycles success
BTC_BLOCKSPACE 59,479 ✅
EL_BLOCK 98,420 ✅
CL_BLOCK 73,165 ✅
CHECKPOINT 2,616 ✅

storopoli
storopoli reviewed Mar 4, 2025
View reviewed changes
Copy link
Member

@storopoli storopoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be really nice if you could in this PR fix that these should all be hardened:

strata/crates/primitives/src/constants.rs

Line 88 in de3a4ff

ChildNumber::from_normal_idx(STRATA_OPERATOR_MESSAGE_IDX)

strata/crates/primitives/src/constants.rs

Line 103 in de3a4ff

ChildNumber::from_normal_idx(STRATA_OPERATOR_WALLET_IDX)

But the datatool explodes if we do this because it converts xpub -> xpub and that fails if is hardened.
This is the bridge spec that we're using, all paths should be hardened.

@delbonis delbonis merged commit 2f2eace into main Mar 12, 2025
18 of 19 checks passed
@delbonis delbonis deleted the STR-482-xpriv-path-cleanup branch March 12, 2025 15:26
@delbonis
Copy link
Contributor Author

Force merging due to triviality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@storopoli storopoli storopoli left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@delbonis @storopoli